Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Command#report_warnings to gracefully handle missing Compiler#program #9866

Merged
merged 1 commit into from
Nov 10, 2020
Merged

Fix Command#report_warnings to gracefully handle missing Compiler#program #9866

merged 1 commit into from
Nov 10, 2020

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Oct 30, 2020 •
edited
Loading

If the compiler was not setup correctly and program is missing, there are no warnings to show and the method should just return gracefully instead of causing a NilAssertion error.

/cc #9789 (comment)

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels Oct 30, 2020
This was referenced Oct 30, 2020
Closed
Closed
@straight-shoota straight-shoota added this to the 1.0.0 milestone Nov 8, 2020
bcardiff
bcardiff reviewed Nov 9, 2020
View reviewed changes
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more correct to show a STDERR at least.

If the report_warnings is called and the compiler.program is not setup with the proposed change it is harder to notice it.

I'm not even sure this change at all is needed. It's a nilable property that we expect to be set when reporting warnings. If it's not set there is somethings else wrong, better crash or signal the error somehow.

@straight-shoota
Copy link
Member Author

If it's not set there is somethings else wrong, better crash or signal the error somehow.

Exactly that's the case. This change (or something alike) is necessary to make the original error show up. Currently it's shadowed by the NilAssertion error. See #9789 (comment) for details.

IMO this change is fine as it is. This condition only occures when there's already some error handling happening. Notifying about program being nil is completely unnecessary at this point because it's just a secondary error.
Two lines above is an return if @compiler has no value. So calling report_warnings doesn't require a compiler to be set up.

@bcardiff
Copy link
Member

ok, thanks for clarifying.

@bcardiff bcardiff merged commit f3d36a6 into crystal-lang:master Nov 10, 2020
@straight-shoota straight-shoota deleted the fix/report_warnings_missing_program branch November 10, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardiff bcardiff bcardiff left review comments

@jhass jhass jhass approved these changes

Assignees
No one assigned
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler
Projects
None yet
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.
3 participants
@straight-shoota @bcardiff @jhass